Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Sprout Join Split validation by transaction verifier #2371

Merged

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Jun 23, 2021

Motivation

As part of NU5, there is a new transaction format and consensus rules (version 5). Zebra already had initial support for the new transaction, but the consensus rules aren't fully implemented. In order to properly validate the new transaction version, some validation code will be shared between V4 and V5 transactions, so this PRs refactors part of it to improve code sharing.

This is the third PR that's part of #1981.

Specifications

The new transaction encoding and consensus rules are detailed in the specification.

Designs

Solution

A new verify_sprout_shielded_data method is created by this PR, and the verify_v4_transaction uses it.

Two tests were also added. Since for now Zebra doesn't fully validate Sprout join splits, the tests only check if the transaction verifier checks the join split data signature.

Review

@teor2345 reviewed previous PRs part of #1981

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

  • Improve TransactionError downcasting?

jvff added 3 commits June 23, 2021 01:55
Move the join split verification code into a new
`verify_sprout_shielded_data` helper method that returns an
`AsyncChecks` set.
Create a fake V4 transaction with a dummy join split, and sign it
appropriately. Check if the transaction verifier accepts the
transaction.
Create a fake V4 transaction with a dummy join split. Do NOT sign this
transaction's join split data, and check that the verifier rejects the
transaction.
@jvff jvff requested a review from teor2345 June 23, 2021 02:03
@jvff jvff self-assigned this Jun 23, 2021
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good so far, happy to re-review once the tests pass.

@jvff jvff force-pushed the refactor-sprout-joinsplit-validation branch 5 times, most recently from 12a19ec to 789f10e Compare June 24, 2021 16:08
Otherwise one of the tests might fail incorrectly because of a
limitation in the test environment. `Batch` services spawn a task in the
Tokio runtime, but separate tests can have separate runtimes, so sharing
a `Batch` service can lead to the worker task only being available for
one of the tests.
@jvff
Copy link
Contributor Author

jvff commented Jun 24, 2021

I have finally been able to work around the issue with the tests that appeared in CI. The work-around is quite brittle, so it might become necessary to find an actual fix soon.

As far as I understood, the issue arises as a consequence of three things:

  • tower_batch::Batch::new spawns a task in the Tokio runtime;
  • the primitives::*::Verifiers are stored in shared global variables, properly handled by Lazy wrappers;
  • #[tokio::test] creates separate runtimes for separate tests

This means that shared batch verifiers will spawn a task on the runtime that first uses it, but that task will not survive for the next runtime (i.e., the next test execution), causing it to fail.

The workaround included here was to merge the two tests into a single test, so that they share the same runtime. This allows the spawned task to live enough for both calls to the verifier.

This workaround is brittle because it means that as more tests are added, more "test merging" will have to be done to avoid the underlying issue. I opened #2390 to track a more permanent solution.

@teor2345
Copy link
Contributor

This workaround is brittle because it means that as more tests are added, more "test merging" will have to be done to avoid the underlying issue. I opened #2390 to track a more permanent solution.

I also added another idea to this ticket: create a shared test verifier tokio runtime.

We already have once-off logging init in the zebra_test crate. It would be easy to add another function for a shared tokio runtime.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just committed a ticket number fix, now we're good to merge.

@teor2345 teor2345 enabled auto-merge (squash) June 25, 2021 00:30
@teor2345 teor2345 merged commit fdeb6d5 into ZcashFoundation:main Jun 25, 2021
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mpguerra mpguerra linked an issue Jun 29, 2021 that may be closed by this pull request
9 tasks
@jvff jvff deleted the refactor-sprout-joinsplit-validation branch November 23, 2021 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Transparent and Sapling validation for transaction v5
3 participants